FABGW-20 Specify endorsing organizations #2578

andrew-coleman · 2021-05-07T09:56:10Z

Implementation of endorsingOrgs in gateway Endorse method.

The set of endorsing peers from the discovery service is filtered down to only include those from the specified orgs.
The discovery handling code has been upgraded to step through all given layouts to find a layout that satisfies the endorsingOrgs constraint.

Signed-off-by: andrew-coleman [email protected]

andrew-coleman · 2021-05-07T11:19:13Z

/ci-run

manish-sethi · 2021-05-07T19:40:07Z

internal/pkg/gateway/registry.go

+	endorser *endorser
 	endpoint string


As you introduced endorser, can we remove endpoint and use the endorser.address instead? Same info in multiple form cause confusion.

Good spot. Removed.

manish-sethi · 2021-05-07T20:53:16Z

internal/pkg/gateway/registry.go

 			peerGroup := groupPeers[0:quantity]
 			r = append(r, peerGroup...)


Not related to this PR but a minor suggestion - A single letter variable works fine if the life of the variable spans a few lines (e.g., peerGroup). The variable r is declared far above from where it is used so a meaningful name would be better.

Suggested change

peerGroup := groupPeers[0:quantity]

r = append(r, peerGroup...)

r = append(r, groupPeers[0:quantity]...)

manish-sethi · 2021-05-07T21:09:24Z

internal/pkg/gateway/registry.go

-			} else {
-				reg.logger.Warnf("Failed to find endorser at %s", peer.endpoint)
-			}
+			endorsers = append(endorsers, peer.endorser)


Just a clarification related to discovery - I assume that the assumption here is that a peer does not appear in more than one groups for a given layout? Or this final slice can contain duplicates?

That's my assumption too. I followed the usage algorithm documented in the proto file:
https://github.com/hyperledger/fabric-protos/blob/main/discovery/protocol.proto#L175
My observation is that each group contains the peers of a single org, and the layouts give the various permutations of those orgs/groups

manish-sethi · 2021-05-07T21:59:00Z

internal/pkg/gateway/registry.go

+				if len(endorsingOrgs) == 0 || contains(endorsingOrgs, endorser.mspid) {
+					groupPeers = append(groupPeers, &endorserState{peer: peer, endorser: endorser, endpoint: endpoint, height: height})


I guess that we should discuss the semantics of user supplied endorsingOrgs little more clearly. The endorsement policies are specified at the chaincode level and could possibly be overwritten at the collection level. In addition, my understanding was that the purpose of this third option was to allow apps to specify the names of the orgs explicitly and this option (instead of knowing the names of the collections) would be helpful for some apps . In other words, the app explicitly supplies an AND policy and the assumption is that the app knows that the supplied AND policy is a superset of the union of all policies for the collections that the transaction writes into. However, the semantics of the implementation here is that the app is supposed to supply an AND policy that we would be intersect with chaincode level policy.

The endorsers service discovery query takes into consideration both the chaincode level and collection level endorsement policy. But Manish is correct, we need to discuss the semantics of endorsingOrgs.

I think the primary use case is when the application knows exactly which orgs to target. In that case, the Service Discovery peers query should be utilized, rather than the more expensive endorsers query. This will often be the case when using state-based endorsement (as SD endorsers query doesn't consider state-based endorsement requirements). See more history in https://jira.hyperledger.org/browse/FABN-1601.

Another use case is when the endorsement policy should be used (for example 3 out of 100 orgs), but you want to target some specific orgs, for example you want to utilize OrgA and OrgB but let service discovery pick any third org.

Bret implemented both approaches for the node SDK, and I think we have to do the same for Gateway.

A note on the endorsers query org filtering approach... Bret used a client side endorsers query filter in node SDK , but that approach doesn't work at all when there are many layouts returned since service discovery may only return a random subset. That means you may entirely miss on the intersection of layouts returned and orgs required. The filter therefore must be done on the service discovery side - we need to consider extending the SD endorsers query to take the list of required orgs. In fact, this is why we have a single team responsible for Gateway and Service Discovery now.

I've reimplemented this as Dave suggests to return all the peers from the list of 'endorsingOrganizations' that have the given chaincode installed.

manish-sethi · 2021-05-12T15:11:55Z

internal/pkg/gateway/registry.go

+	return nil, fmt.Errorf("failed to select a set of endorsers that satisfy the endorsement policy")
+}
+
+// endorsersForOrgs returns a set of endorsers oen by the given orgs for the given chaincode on a channel.


what does "oen" mean?

manish-sethi · 2021-05-12T15:42:40Z

internal/pkg/gateway/registry.go

+	}
+
+	var endorsers []*endorser
+	members := reg.discovery.PeersOfChannel(common.ChannelID(channel))


Nothing to be done in this PR, However, just to take a note, this appears to be case to revisit if the discovery api/code could be extended to take the org names as a filter that could improve the efficiency of this as compared to get all the peers and iterate over to filter.

manish-sethi · 2021-05-12T15:45:19Z

internal/pkg/gateway/registry.go

+			for _, installedChaincode := range member.Properties.Chaincodes {
+				// only consider the peers that have our chaincode installed
+				if installedChaincode.Name == chaincode {
+					endorsers = append(endorsers, endorser)


Like the existing function, I was expecting here as well - only one peer (with the maximum height) for one org. Am I missing something?

manish-sethi · 2021-05-21T15:09:47Z

internal/pkg/gateway/registry.go

+		if len(es) > 0 {
+			endorsers = append(endorsers, es[0].endorser)
+		}


Isn't it a noop check?

If, for any reason, the endorser returned by the discovery service is not in the gateway registry, then this could be an empty array. Perhaps if a new endorser had just come online and not been added to the registry yet. There's still a TODO comment in registerChannels() to handle this type of dynamic update.

Discussed offline and @andrew-coleman is going to push a new patch. Holding it from merging for now.

Thanks Manish, I've added an error check that will report which orgs fail to find endorsing peers, and added extra unit tests

manish-sethi

Based on off-line discussion, expecting a minor fix and test for handling the case of insufficient available peers

Implementation of endorsingOrgs in gateway Endorse method. - The set of endorsing peers from the discovery service is filtered down to only include those from the specified orgs. - The discovery handling code has been upgraded to step through all given layouts to find a layout that satisfies the endorsingOrgs constraint. Signed-off-by: andrew-coleman <[email protected]>

andrew-coleman requested a review from a team as a code owner May 7, 2021 09:56

manish-sethi reviewed May 7, 2021

View reviewed changes

andrew-coleman force-pushed the main branch 2 times, most recently from 81c1842 to 7801a1f Compare May 12, 2021 08:31

andrew-coleman requested review from manish-sethi and denyeart May 12, 2021 09:18

andrew-coleman force-pushed the main branch from 7801a1f to 6d57e51 Compare May 12, 2021 09:30

manish-sethi reviewed May 12, 2021

View reviewed changes

andrew-coleman force-pushed the main branch from 6d57e51 to 0eb43df Compare May 20, 2021 10:15

andrew-coleman requested a review from manish-sethi May 21, 2021 13:22

manish-sethi reviewed May 21, 2021

View reviewed changes

manish-sethi approved these changes May 21, 2021

View reviewed changes

manish-sethi requested changes May 21, 2021

View reviewed changes

andrew-coleman force-pushed the main branch from 0eb43df to 8478570 Compare May 21, 2021 15:56

andrew-coleman requested a review from manish-sethi May 21, 2021 16:00

manish-sethi approved these changes May 21, 2021

View reviewed changes

manish-sethi merged commit b4efe85 into hyperledger:main May 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FABGW-20 Specify endorsing organizations #2578

FABGW-20 Specify endorsing organizations #2578

andrew-coleman commented May 7, 2021

andrew-coleman commented May 7, 2021

manish-sethi May 7, 2021

andrew-coleman May 12, 2021

manish-sethi May 7, 2021

manish-sethi May 7, 2021

andrew-coleman May 12, 2021

manish-sethi May 7, 2021

denyeart May 8, 2021

andrew-coleman May 12, 2021

manish-sethi May 12, 2021

manish-sethi May 12, 2021

manish-sethi May 12, 2021

manish-sethi May 21, 2021 •

edited

Loading

andrew-coleman May 21, 2021

manish-sethi May 21, 2021

andrew-coleman May 21, 2021

manish-sethi left a comment

		peerGroup := groupPeers[0:quantity]
		r = append(r, peerGroup...)

	peerGroup := groupPeers[0:quantity]
	r = append(r, peerGroup...)
	r = append(r, groupPeers[0:quantity]...)

		if len(endorsingOrgs) == 0 \|\| contains(endorsingOrgs, endorser.mspid) {
		groupPeers = append(groupPeers, &endorserState{peer: peer, endorser: endorser, endpoint: endpoint, height: height})

FABGW-20 Specify endorsing organizations #2578

FABGW-20 Specify endorsing organizations #2578

Conversation

andrew-coleman commented May 7, 2021

andrew-coleman commented May 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manish-sethi May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manish-sethi left a comment

Choose a reason for hiding this comment

manish-sethi May 21, 2021 •

edited

Loading